fix(mcp): strip root_path before matching the per-server MCP route spelling - #35576
fix(mcp): strip root_path before matching the per-server MCP route spelling#35576tin-berri wants to merge 1 commit into
Conversation
…elling
The 401 challenge for a gateway-managed oauth2 MCP server advertises the
protected-resource metadata URL in the spelling the client connected on, so a
strict RFC 9728 section 3 client lands on a document whose `resource` equals the
URL it actually called. That spelling test compared `_original_path` against the
root-relative `/{server}/mcp` shape, but `_original_path` and `scope["path"]`
are raw request-line paths that still carry the deployment's `root_path`
On a SERVER_ROOT_PATH deployment the prefix therefore made the legacy test fail
and every request fell through to the standard `/mcp/{server}` branch. A client
connecting on `/litellm/github/mcp` was pointed at the standard-pattern
document, which serves `resource = {base}/litellm/mcp/github`; that is not the
URL the client called, so a strict client aborts discovery before the MCP
request fires
Route the path through `get_route_relative_request_path` first, which removes
`root_path` on a segment boundary the same way
`litellm.proxy.auth.auth_utils.get_request_route` already does for the rest of
the MCP auth path, so `/litellmfoo` is not truncated under `root_path=/litellm`
Greptile SummaryThe PR normalizes raw MCP request paths against the deployment root path before selecting the per-server OAuth discovery spelling
Confidence Score: 5/5The PR appears safe to merge; the normalization matches the production scope rewrite and preserves both supported MCP route spellings The helper strips only an exact root-path prefix or a prefix followed by a segment boundary, and the regression test exercises the real challenge-building caller for both affected route forms
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/oauth_utils.py | Adds narrowly scoped root-path normalization before the existing route-spelling comparison, preserving root-mounted behavior and segment boundaries |
| tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py | Adds focused regression coverage through process_mcp_request for both supported per-server spellings under a deployment root path |
Reviews (1): Last reviewed commit: "fix(mcp): strip root_path before matchin..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| return "" if root == "/" else root | ||
|
|
||
|
|
||
| def get_route_relative_request_path(scope: Scope) -> str: |
There was a problem hiding this comment.
the authorization_uri branch in server.py picks its /mcp/{server_name} vs /{server_name} well-known shape off the same raw scope["_original_path"] (server.py:3767), so under SERVER_ROOT_PATH it takes the else-branch for both spellings and hands back /.well-known/oauth-authorization-server/{server}. Same mismatch this fixes, just the gateway-managed authorization_code path instead of the passthrough one — worth routing that one through get_route_relative_request_path too.
TLDR
Problem this solves:
_original_pathagainst the root-relative/{server}/mcpshape_original_path(andscope["path"]) are raw request-line paths, so on aSERVER_ROOT_PATHdeployment they still carry the prefix and that match always fails/litellm/{server}/mcpis therefore pointed at the standard-pattern discovery document, whoseresourceis{base}/litellm/mcp/{server}rather than the URL it called, and a strict RFC 9728 section 3 client aborts before the MCP request firesHow it solves it:
root_pathfrom the path before the spelling match, on a segment boundary, the same waylitellm.proxy.auth.auth_utils.get_request_routealready does for the rest of the MCP auth pathroot_pathRelevant issues
SERVER_ROOT_PATH(sub-path) deploymentresourcea client discovers now equals the MCP URL it connected to, in both the/{server}/mcpand/mcp/{server}spellingsSERVER_ROOT_PATHthrough the realprocess_mcp_requestcallerSurfaced by the discussion on #35226, which reported the same class of
resourcemismatch. That PR proposes a new opt-in env var to derive the discovery path from the request; this change instead fixes the root-path normalization the existing code already relies on, which covers the sub-path deployment without new configurationLinear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Live proxy on a sub-path deployment, config below, started with
Before (this branch's base,
ba480a619f) both spellings collapse onto the standard-pattern document:Following the challenge a
/litellm/github/mcpclient was sent to shows the mismatch it aborts on:After (this branch) each spelling keeps its own document:
and each document's
resourceis exactly the URL the client connected to:Type
🐛 Bug Fix
Changes
oauth_utils.pygainsget_route_relative_request_path, which reads_original_path(falling back toscope["path"]) and removes the deployment'sroot_pathwhen the raw path is that prefix or continues past it on a/boundary, so/litellmfoois not truncated underroot_path=/litellm.get_passthrough_resource_metadata_urlnow compares that normalized path instead of the raw oneNothing else changes. On a root-mounted proxy
root_pathis empty and the helper returns the raw path unchanged, so the emitted metadata URL is byte-identical to today'sThe regression test lives in the existing
TestAggregateGatewayDcrChallengeclass next to the non-root-path spelling test it mirrors, and drives the realprocess_mcp_requestentry point rather than the helper, so it fails if either the challenge or the spelling selection regresses. It reverts to the pre-fix assertion failure when the normalization is removedQA runbook
uv run --no-sync pytest tests/test_litellm/proxy/_experimental/mcp_server/auth/ tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py -qmcp_serversconfig above tomcp_rootpath_config.yamlSERVER_ROOT_PATH=/litellm LITELLM_MASTER_KEY=sk-1234 python litellm/proxy/proxy_cli.py --config mcp_rootpath_config.yaml --port 4111curlloops from the proof section and confirm the two spellings now advertise differentresource_metadataURLsGETeach advertised URL and confirmresourceequals the MCP URL from step 4SERVER_ROOT_PATHunset and confirm the challenges are unchanged from before this PR (/.well-known/oauth-protected-resource/github/mcpand/.well-known/oauth-protected-resource/mcp/github)Final Attestation